[SPARK-30821][K8S]Handle executor failure with multiple containers #29924

huskysun · 2020-10-01T17:29:02Z

Handle executor failure with multiple containers

Added a spark property spark.kubernetes.executor.checkAllContainers,
with default being false. When it's true, the executor snapshot will
take all containers in the executor into consideration when deciding
whether the executor is in "Running" state, if the pod restart policy is
"Never". Also, added the new spark property to the doc.

What changes were proposed in this pull request?

Checking of all containers in the executor pod when reporting executor status, if the spark.kubernetes.executor.checkAllContainers property is set to true.

Why are the changes needed?

Currently, a pod remains "running" as long as there is at least one running container. This prevents Spark from noticing when a container has failed in an executor pod with multiple containers. With this change, user can configure the behavior to be different. Namely, if any container in the executor pod has failed, either the executor process or one of its sidecars, the pod is considered to be failed, and it will be rescheduled.

Does this PR introduce any user-facing change?

Yes, new spark property added.
User is now able to choose whether to turn on this feature using the spark.kubernetes.executor.checkAllContainers property.

How was this patch tested?

Unit test was added and all passed.
I tried to run integration test by following the instruction here (section "Testing K8S") and also here, but I wasn't able to run it smoothly as it fails to talk with minikube cluster. Maybe it's because my minikube version is too new (I'm using v1.13.1)...? Since I've been trying it for two days and still can't make it work, I decided to submit this PR and hopefully the Jenkins test will pass.

Added a spark property spark.kubernetes.executor.checkAllContainers, with default being false. When it's true, the executor snapshot will take all containers in the executor into consideration when deciding whether the executor is in "Running" state, if the pod restart policy is "Never". Also, added the new spark property to the doc.

huskysun · 2020-10-01T17:33:23Z

This PR is a continuation of the effort for #27568.
@holdenk Please take a look, as you said you're interested in merging this. (Sorry for taking this long to submit the PR 😅 )
cc @gongx

gongx · 2020-10-13T18:13:03Z

@holdenk Could you review this PR, please?

holdenk

Jenkins ok to test. Sorry for the delay. tentative LGTM pending CI/jenkins.

...rnetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsSnapshot.scala

holdenk · 2020-10-13T18:17:30Z

...rnetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsSnapshot.scala

@@ -59,11 +65,19 @@ object ExecutorPodsSnapshot extends Logging {
        case "pending" =>
          PodPending(pod)
        case "running" =>
-          PodRunning(pod)
+          if (shouldCheckAllContainers &&
+            "Never" == pod.getSpec.getRestartPolicy &&


Good addition 👍

huskysun · 2020-10-13T19:20:24Z

@holdenk Made the fix about the indentation. Please take a look again. Thanks!

huskysun · 2020-10-13T19:28:39Z

Also is Jenkins triggered? Looks like the bot didn't make any comment to this PR.

holdenk · 2020-10-13T20:28:03Z

Jenkins OK to test

holdenk · 2020-10-14T18:28:07Z

huh weird. Jenkins test this please.

holdenk · 2020-10-14T18:32:44Z

cc @SparkQA
Jenkins OK to test

holdenk · 2020-10-14T18:38:06Z

Filed a ticket to @shaneknapp ( https://issues.apache.org/jira/browse/SPARK-33151 ) since it seems like Jenkins might just be stuck.

gongx · 2020-10-14T18:41:47Z

@holdenk Thank you for the help. I do see that the Jenkins status is at "Asked to test", but it does not make any progress.

SparkQA · 2020-10-14T21:29:48Z

Test build #129762 has finished for PR 29924 at commit a4958b9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-14T23:28:08Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34368/

SparkQA · 2020-10-14T23:46:12Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34368/

holdenk

LGTM. If no one has concerns I'll try and merge tomorrow. Thanks for working on this @huskysun :)

huskysun · 2020-10-15T17:51:46Z

Thank you @holdenk!
Also thanks @khogeland for starting on this effort and @gongx for the help on coordination.

gongx · 2020-10-16T21:19:54Z

@holdenk Thank you for the review. Please merge it if you get time.

gongx · 2020-10-20T17:16:10Z

@holdenk Please help us to merge this change if you have time. Thank you.

huskysun · 2020-10-23T18:03:32Z

Hi @holdenk sorry for keeping bugging you. Any updates on this? Is there any further work needed behind the curtain (like release cadence or something) that prevents this from being merged? Thanks!

dongjoon-hyun · 2020-10-23T20:36:07Z

Retest this please.

SparkQA · 2020-10-23T20:50:27Z

Test build #130215 has finished for PR 29924 at commit a4958b9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-23T21:20:45Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34815/

SparkQA · 2020-10-23T21:49:25Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34815/

holdenk · 2020-10-24T16:58:06Z

Meregd and backported as a fix to branch-3.

holdenk · 2020-10-24T17:01:21Z

@huskysun what is your JIRA account handle?

Handle executor failure with multiple containers Added a spark property spark.kubernetes.executor.checkAllContainers, with default being false. When it's true, the executor snapshot will take all containers in the executor into consideration when deciding whether the executor is in "Running" state, if the pod restart policy is "Never". Also, added the new spark property to the doc. ### What changes were proposed in this pull request? Checking of all containers in the executor pod when reporting executor status, if the `spark.kubernetes.executor.checkAllContainers` property is set to true. ### Why are the changes needed? Currently, a pod remains "running" as long as there is at least one running container. This prevents Spark from noticing when a container has failed in an executor pod with multiple containers. With this change, user can configure the behavior to be different. Namely, if any container in the executor pod has failed, either the executor process or one of its sidecars, the pod is considered to be failed, and it will be rescheduled. ### Does this PR introduce _any_ user-facing change? Yes, new spark property added. User is now able to choose whether to turn on this feature using the `spark.kubernetes.executor.checkAllContainers` property. ### How was this patch tested? Unit test was added and all passed. I tried to run integration test by following the instruction [here](https://spark.apache.org/developer-tools.html) (section "Testing K8S") and also [here](https://github.com/apache/spark/blob/master/resource-managers/kubernetes/integration-tests/README.md), but I wasn't able to run it smoothly as it fails to talk with minikube cluster. Maybe it's because my minikube version is too new (I'm using v1.13.1)...? Since I've been trying it for two days and still can't make it work, I decided to submit this PR and hopefully the Jenkins test will pass. Closes #29924 from huskysun/exec-sidecar-failure. Authored-by: Shiqi Sun <s.sun@salesforce.com> Signed-off-by: Holden Karau <hkarau@apple.com> (cherry picked from commit f659527) Signed-off-by: Holden Karau <hkarau@apple.com>

huskysun · 2020-10-25T02:35:27Z

Thanks for merging it in! I just created a JIRA account, and my username is "huskysun" and full name is "Shiqi Sun". Let me know if these work, thanks.

…

On Sat, Oct 24, 2020 at 10:01 AM Holden Karau ***@***.***> wrote: @huskysun <https://github.com/huskysun> what is your JIRA account handle? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29924 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFON6VOGD3WQKW6EEYXLYHLSMMB65ANCNFSM4SAW6Z5A> .

Handle executor failure with multiple containers Added a spark property spark.kubernetes.executor.checkAllContainers, with default being false. When it's true, the executor snapshot will take all containers in the executor into consideration when deciding whether the executor is in "Running" state, if the pod restart policy is "Never". Also, added the new spark property to the doc. ### What changes were proposed in this pull request? Checking of all containers in the executor pod when reporting executor status, if the `spark.kubernetes.executor.checkAllContainers` property is set to true. ### Why are the changes needed? Currently, a pod remains "running" as long as there is at least one running container. This prevents Spark from noticing when a container has failed in an executor pod with multiple containers. With this change, user can configure the behavior to be different. Namely, if any container in the executor pod has failed, either the executor process or one of its sidecars, the pod is considered to be failed, and it will be rescheduled. ### Does this PR introduce _any_ user-facing change? Yes, new spark property added. User is now able to choose whether to turn on this feature using the `spark.kubernetes.executor.checkAllContainers` property. ### How was this patch tested? Unit test was added and all passed. I tried to run integration test by following the instruction [here](https://spark.apache.org/developer-tools.html) (section "Testing K8S") and also [here](https://github.com/apache/spark/blob/master/resource-managers/kubernetes/integration-tests/README.md), but I wasn't able to run it smoothly as it fails to talk with minikube cluster. Maybe it's because my minikube version is too new (I'm using v1.13.1)...? Since I've been trying it for two days and still can't make it work, I decided to submit this PR and hopefully the Jenkins test will pass. Closes apache#29924 from huskysun/exec-sidecar-failure. Authored-by: Shiqi Sun <s.sun@salesforce.com> Signed-off-by: Holden Karau <hkarau@apple.com> (cherry picked from commit f659527) Signed-off-by: Holden Karau <hkarau@apple.com>

…rs` by default ### What changes were proposed in this pull request? This PR aims to enable `spark.kubernetes.executor.checkAllContainers` by default from Apache Spark 4.0.0. ### Why are the changes needed? Since Apache Spark 3.1.0, `spark.kubernetes.executor.checkAllContainers` is supported and useful because [sidecar pattern](https://kubernetes.io/docs/concepts/workloads/pods/sidecar-containers/) is used in many cases. Also, it prevents user mistakes which forget and ignore the sidecars' failures by always reporting sidecar failures via executor status. - #29924 ### Does this PR introduce _any_ user-facing change? - This configuration is no-op when there is no other container. - This will report user containers' error correctly when there exist other containers which are provided by the users. ### How was this patch tested? Both `true` and `false` are covered by our CI test coverage since Apache Spark 3.1.0. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47337 from dongjoon-hyun/SPARK-48887. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

probot-autolabeler bot added DOCS KUBERNETES labels Oct 1, 2020

holdenk reviewed Oct 13, 2020

View reviewed changes

Fix indentation to match the format

a4958b9

holdenk approved these changes Oct 15, 2020

View reviewed changes

asfgit closed this in f659527 Oct 24, 2020

dongjoon-hyun mentioned this pull request Jul 12, 2024

[SPARK-48887][K8S] Enable spark.kubernetes.executor.checkAllContainers by default #47337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30821][K8S]Handle executor failure with multiple containers #29924

[SPARK-30821][K8S]Handle executor failure with multiple containers #29924

huskysun commented Oct 1, 2020

huskysun commented Oct 1, 2020

gongx commented Oct 13, 2020

holdenk left a comment

holdenk Oct 13, 2020

huskysun commented Oct 13, 2020

huskysun commented Oct 13, 2020

holdenk commented Oct 13, 2020

holdenk commented Oct 14, 2020

holdenk commented Oct 14, 2020

holdenk commented Oct 14, 2020

gongx commented Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

holdenk left a comment

huskysun commented Oct 15, 2020

gongx commented Oct 16, 2020

gongx commented Oct 20, 2020

huskysun commented Oct 23, 2020

dongjoon-hyun commented Oct 23, 2020

SparkQA commented Oct 23, 2020

SparkQA commented Oct 23, 2020

SparkQA commented Oct 23, 2020

holdenk commented Oct 24, 2020

holdenk commented Oct 24, 2020

huskysun commented Oct 25, 2020 via email

[SPARK-30821][K8S]Handle executor failure with multiple containers #29924

[SPARK-30821][K8S]Handle executor failure with multiple containers #29924

Conversation

huskysun commented Oct 1, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

huskysun commented Oct 1, 2020

gongx commented Oct 13, 2020

holdenk left a comment

Choose a reason for hiding this comment

holdenk Oct 13, 2020

Choose a reason for hiding this comment

huskysun commented Oct 13, 2020

huskysun commented Oct 13, 2020

holdenk commented Oct 13, 2020

holdenk commented Oct 14, 2020

holdenk commented Oct 14, 2020

holdenk commented Oct 14, 2020

gongx commented Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

holdenk left a comment

Choose a reason for hiding this comment

huskysun commented Oct 15, 2020

gongx commented Oct 16, 2020

gongx commented Oct 20, 2020

huskysun commented Oct 23, 2020

dongjoon-hyun commented Oct 23, 2020

SparkQA commented Oct 23, 2020

SparkQA commented Oct 23, 2020

SparkQA commented Oct 23, 2020

holdenk commented Oct 24, 2020

holdenk commented Oct 24, 2020

huskysun commented Oct 25, 2020 via email